Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete respect parent api #1257

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete respect parent api #1257

merged 6 commits into from
Oct 11, 2022

Conversation

midlan
Copy link
Contributor

@midlan midlan commented Oct 11, 2020 •
edited
Loading

Description (*)

I fixed overrides of Mage_Core_Model_Resource_Db_Abstract::delete to respect parent api, which means:

  • throw on error
  • return $this

Fixed Issues (if relevant)

  1. Fixes No throw when Mage_Admin_Model_User::delete fails #950

Manual testing scenarios (*)

Admin panel

  1. make some constraint for admin_user table that not allow you delete record (e.g. foreign key referencing the table with RESTRICT rule ON DELETE)
  2. go to admin panel and try delete some user, instead of green message that user was deleted, we should get red error message

API

There is changes also in API resource model, so we should test it also.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api Component: Core Relates to Mage_Core labels Oct 11, 2020
kkrieger85
kkrieger85 previously approved these changes Oct 20, 2020
View reviewed changes
@midlan
Copy link
Contributor Author

midlan commented Mar 4, 2021

@Flyingmana @sreichel review pls?

@midlan midlan added the backwards compatibility Might affect backwards compatibility for some users label Aug 24, 2021
@midlan midlan changed the base branch from 1.9.4.x to 20.0 August 24, 2021 08:54
@midlan midlan dismissed kkrieger85’s stale review August 24, 2021 08:54

The base branch was changed.

@midlan
Copy link
Contributor Author

midlan commented Aug 24, 2021

It is BC break, labeled it and changed base branch to 20.0

Sekiphp
Sekiphp previously approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@Sekiphp Sekiphp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems ok

for future compatibility, you should consider to replace catch of Exception to Throwable

@midlan midlan added the review needed Problem should be verified label Dec 23, 2021
@fballiano
Copy link
Contributor

seems ok

for future compatibility, you should consider to replace catch of Exception to Throwable

I switched them

fballiano
fballiano previously approved these changes Jun 15, 2022
View reviewed changes
@fballiano
Copy link
Contributor

But we should still add a note to the README in the "differences between v19 and v20"

@elidrissidev
Copy link
Member

Updated the branch since the README's version was ancient and added the note to the README.

@elidrissidev elidrissidev changed the title Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete to respect parent api Make overrides of Mage_Core_Model_Resource_Db_Abstract::delete respect parent api Oct 11, 2022
@elidrissidev elidrissidev removed the review needed Problem should be verified label Oct 11, 2022
@elidrissidev elidrissidev merged commit 61f1a22 into OpenMage:20.0 Oct 11, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  7 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 61f1a22. ± Comparison against base commit bc4b2cd.

@sreichel sreichel mentioned this pull request Dec 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elidrissidev elidrissidev elidrissidev approved these changes

@kiatng kiatng kiatng approved these changes

@Sekiphp Sekiphp Sekiphp left review comments

@fballiano fballiano fballiano left review comments

@kkrieger85 kkrieger85 Awaiting requested review from kkrieger85

@Flyingmana Flyingmana Awaiting requested review from Flyingmana

Assignees
No one assigned
Labels
backwards compatibility Might affect backwards compatibility for some users Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api Component: Core Relates to Mage_Core documentation hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No throw when Mage_Admin_Model_User::delete fails
7 participants
@midlan @fballiano @elidrissidev @kiatng @kkrieger85 @Sekiphp @Flyingmana